Add independent ciper and MAC algorithms negotiation for each direction by yosuke-wolfssl · Pull Request #952 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-04-24T00:22:56Z

This PR adds independent ciper and MAC algorithms negotiation for each direction to comply with RFC 4253 section 7.1.

Add peerEncryptId, peerMacId, peerAeadMode, peerBlockSz, peerMacSz into HandshakeInfo.
Fix DoKexInit() so that this matches each algorithm independently and store the results into each member in HandshakeInfo.
Fix DoNewKeys() so that this updates peer algorithm info like ssh->peerEncryptId with ssh->handshake->peerEncryptId and so on.

Also, new regress test for the case is added as well.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds RFC 4253 §7.1-compliant, per-direction cipher/MAC negotiation by tracking and applying independent peer vs local (C2S vs S2C) algorithm selections during KEXINIT/NEWKEYS processing, with a new regression test covering mixed-direction negotiation scenarios.

Changes:

Extend HandshakeInfo to store peer (opposite direction) cipher/MAC/AEAD and sizes independently.
Update DoKexInit() to negotiate cipher/MAC independently per direction and populate the new handshake fields.
Update DoNewKeys() to copy negotiated peer-direction algorithm selections from the handshake into the session, and add a regression test validating negotiation outcomes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`wolfssh/internal.h`	Adds per-direction (peer) negotiated cipher/MAC/AEAD fields to `HandshakeInfo`.
`src/internal.c`	Implements independent per-direction algorithm matching and propagates peer settings on `NEWKEYS`.
`tests/regress.c`	Adds a regression test ensuring negotiation differs per direction and handles AEAD vs non-AEAD correctly.

Comments suppressed due to low confidence (1)

src/internal.c:4486

The KEXINIT parsing now duplicates the same 'convert configured algo name-list into ID list' pattern multiple times (cipher S2C, MAC C2S, MAC S2C). Consider extracting a small helper to build cannedList/cannedListSz from ssh->algoListCipher / ssh->algoListMac to reduce repetition and the chance of future divergence between the per-direction negotiation blocks.

    if (ret == WS_SUCCESS && !ssh->handshake->peerAeadMode) {
        cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
        cannedListSz = (word32)sizeof(cannedList);
        ret = GetNameListRaw(cannedList, &cannedListSz,
                (const byte*)ssh->algoListMac, cannedAlgoNamesSz);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

aidangarske

Skoll Code Review

Scan type: review-security
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD — src/internal.c:2696-2709
[Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD — tests/regress.c:2142-2210
[Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) — src/internal.c:572-590

Review generated by Skoll

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

yosuke-wolfssl · 2026-04-27T01:18:30Z

Hello @aidangarske ,

I fixed the issues you mentioned.
Could you please review this again ?
Thank you for your time.

aidangarske

Skoll Multi-Scan Review

Modes: review + review-security + audit
Overall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] [audit] DoKexInit client-side branch (WOLFSSH_ENDPOINT_CLIENT alias mapping) lacks a direct asymmetric-algo test — src/internal.c:4313-4326
[Low] [review+review-security] Asymmetric AEAD reset between C2S and S2C branches in DoKexInit — src/internal.c:4427-4478
[Low] [review-security] Pointer aliases used after first ret==WS_SUCCESS block may trip strict-uninit warnings — src/internal.c:4253-4326,4427-4527
[Info] [review-security] Removed redundant ID_NONE/MSGID_NONE initializations in HandshakeInfoNew rely on enum values being 0 — src/internal.c:572-576

Review generated by Skoll

yosuke-wolfssl · 2026-04-28T02:45:07Z

Hi @aidangarske ,

Sorry to take your time.
Can you review again please ?

aidangarske

Skoll Multi-Scan Review

Modes: audit + review + review-security
Overall recommendation: COMMENT
Findings: 7 total — 7 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] [audit] Asymmetric S2C-only encryption-algo mismatch error path is untested — src/internal.c:4448-4466
[Medium] [audit] Asymmetric S2C-only MAC-algo mismatch error path is untested — src/internal.c:4510-4533
[Medium] [review] TestIndependentAlgoNegotiation tests do not verify all ID/Sz fields are reset on each ssh instance — tests/regress.c:2161-2332
[Low] [audit] wolfSSH_TestGenerateKeys NULL-argument validation is untested — src/internal.c:17962-17967
[Low] [review] Missing blank line between TestGenerateKeysSplit and TestGenerateKeysSplitClient — tests/regress.c:2446-2447
[Low] [review] New tests silently ignore wolfSSH_TestDoKexInit return without explanation — tests/regress.c:2191,2227,2278,2313,2366,2420,2479,2532
[Low] [review] DoKexInit reads side from ssh->ctx->side a second time after caching it in side — src/internal.c:4622,4642,4666,4673

Review generated by Skoll

dgarske · 2026-05-05T18:20:49Z

@yosuke-wolfssl please see the Fenrir feedback.

yosuke-wolfssl · 2026-05-07T05:50:39Z

Hello @aidangarske ,
I also fixed some issues with Skoll now.
Could you please check this ?

ejohnstown · 2026-05-07T20:24:14Z

    word32 cannedAlgoNamesSz;
    word32 skipSz = 0;
    word32 begin;
+    /* handshake->keys/encryptId/... always represent the LOCAL endpoint's


I think this can get solved with adding just a few bytes and not having the pointers. We'd have byte c2sEncryptId, s2cEncryptId, s2cMacId, c2SMacId. Read those values. When we're done reading the message, we can check the side and do all the algorithm IDs or peer algorithm IDs depending on the side. All the other values that are based on the read-from-message IDs can be set as well. We'd also need s2cAeadMode and c2sAeadMode flags.

…on, And add regress test

yosuke-wolfssl self-assigned this Apr 24, 2026

Copilot AI review requested due to automatic review settings April 24, 2026 00:22

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread src/internal.c Outdated

Copilot started reviewing on behalf of yosuke-wolfssl April 24, 2026 00:35 View session

yosuke-wolfssl force-pushed the f_607 branch 2 times, most recently from 5cc90fa to 47c9e57 Compare April 24, 2026 00:59

yosuke-wolfssl assigned wolfSSL-Bot and unassigned yosuke-wolfssl Apr 24, 2026

padelsbach requested a review from wolfSSL-Fenrir-bot April 24, 2026 17:11

wolfSSL-Fenrir-bot reviewed Apr 24, 2026

View reviewed changes

aidangarske reviewed Apr 24, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread tests/regress.c

Comment thread src/internal.c Outdated

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot April 26, 2026 23:08

wolfSSL-Fenrir-bot reviewed Apr 26, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/internal.c Outdated

yosuke-wolfssl commented Apr 26, 2026

View reviewed changes

Comment thread src/internal.c Outdated

yosuke-wolfssl commented Apr 27, 2026

View reviewed changes

Comment thread src/internal.c Outdated

yosuke-wolfssl force-pushed the f_607 branch from 47c9e57 to 81a22a2 Compare April 27, 2026 01:01

yosuke-wolfssl requested a review from wolfSSL-Fenrir-bot April 27, 2026 01:02

wolfSSL-Fenrir-bot reviewed Apr 27, 2026

View reviewed changes

aidangarske reviewed Apr 27, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/internal.c

Comment thread src/internal.c Outdated

Comment thread src/internal.c

yosuke-wolfssl force-pushed the f_607 branch from 81a22a2 to c514c19 Compare April 27, 2026 23:12

aidangarske reviewed Apr 28, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread tests/regress.c

Comment thread src/internal.c Outdated

Comment thread tests/regress.c

Comment thread tests/regress.c

dgarske assigned yosuke-wolfssl May 5, 2026

yosuke-wolfssl force-pushed the f_607 branch 2 times, most recently from 226c39b to f330035 Compare May 7, 2026 05:37

dgarske requested a review from aidangarske May 7, 2026 13:29

dgarske unassigned yosuke-wolfssl May 7, 2026

ejohnstown requested changes May 7, 2026

View reviewed changes

dgarske assigned yosuke-wolfssl May 7, 2026

Add independent ciper and MAC algorithms negotiation for each directi…

b76a03d

…on, And add regress test

yosuke-wolfssl force-pushed the f_607 branch from f330035 to b76a03d Compare May 8, 2026 00:26

Conversation

yosuke-wolfssl commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #952

Uh oh!

yosuke-wolfssl commented Apr 27, 2026

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yosuke-wolfssl commented Apr 28, 2026

Uh oh!

aidangarske left a comment

Choose a reason for hiding this comment

Skoll Multi-Scan Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske commented May 5, 2026

Uh oh!

yosuke-wolfssl commented May 7, 2026

Uh oh!

ejohnstown May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants